Skip to content

Rename field error to execution error; define response position #1152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 17, 2025

Conversation

benjie
Copy link
Member

@benjie benjie commented Mar 27, 2025

This has bugged me for years, but it turns out to be a blocker around disabling error propagation... so I'm proposing we finally fix it.

The GraphQL spec is wrong when it refers to field errors. Case in point:

If all fields from the root of the request to the source of the field error return Non-Null types, then the "data" entry in the response should be null.

This is not true for the schema type Query { foo: [Foo]! } type Foo { throws: Int! } with the query { foo { throws } }. All fields from the root of the request to the source of the field error return Non-Null types, but the list position inside the foo return type is nullable, and thus the error should (and does!) stop at that error boundary, returning { data: {foo: [null]}, errors: [...]} and not { data: null, errors: [...] } as the spec (incorrectly) states.

This seems to be an oversight of the list type throughout the spec; there are a lot of incidences where we refer to errors happening at a "field" when actually we mean they're happening at an errorable position - that is to say, a position that can be identified by the path in the response. Factoring in that this path will also want to be used by incremental delivery, rather than calling it an "error position" I've called it a "response position". I did moot calling it simply "position", which I really like, but we do have instances of referring to argument positions, which are distinct from response positions, so I opted for the longer but less ambiguous name.

To make this clearer I've renamed "field error" to "execution error". Technically all execution errors originate in fields, but not necessarily at the field itself - it could be nested within a list. There are three main sources:

  • Argument coercion - happens at the field
  • Errors thrown by the resolver and caught by GraphQL - happens at the field1
  • Errors occurring during result coercion - happens at the field, and within any nested list positions

I've introduced <a name="..."> tags in the spec text to maintain these key hyperlinks even after changing titles of sections.

I've also fixed a number of bugs, ambiguities, and omissions in the spec relating to error handling.

These all reflect the behavior of GraphQL.js (and, I believe, all other GraphQL implementations) and thus I've marked this as editorial.

@graphql/implementers Do any of your implementations handle errors in the way that the spec currently states, as opposed to the way that the reference implementation works? If so, speak now and we can turn this into an RFC process rather than editorial!

cc @JoviDeCroock @yaacovCR Please confirm that my spec edits correctly represent the way in which GraphQL.js operates.

Footnotes

  1. in spec terms, though in GraphQL.js it's possible to return e.g. [1, new Error("two"), 3] and have the second item be treated as an error thrown in that response position.

@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Mar 27, 2025
Copy link

netlify bot commented Mar 27, 2025

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 7abc413
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/680052dda68f680008b29dbb
😎 Deploy Preview https://deploy-preview-1152--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@benjie benjie changed the title Rename "field error" to "runtime error", and define "response position" Rename field error to execution error; define response position Mar 27, 2025
Comment on lines +519 to +520
may lose data, raising an execution error is more appropriate. For example, a
floating-point number `1.2` should raise an execution error instead of being
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
may lose data, raising an execution error is more appropriate. For example, a
floating-point number `1.2` should raise an execution error instead of being
may lose data, raising an _execution error_ is more appropriate. For example, a
floating-point number `1.2` should raise an _execution error_ instead of being

If a field error is raised while resolving a field, it is handled as though the
field returned {null}, and the error must be added to the {"errors"} list in the
response.
If an execution error is raised while resolving a field (either directly or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do underscores around execution error everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general pattern is to do it at least once per headed section, but not to worry about it after that. I generally do it when I feel it adds clarity.

Copy link
Contributor

@robrichard robrichard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, especially defining response position which improves clarity and will be helpful for Incremental Delivery

robrichard added a commit that referenced this pull request Apr 11, 2025
Extracted from #1152, without renaming field error to execution error

Co-authored-by: Benjie <[email protected]>
Copy link
Collaborator

@leebyron leebyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this change

Comment on lines 629 to 631
Any _request error_ raised during {CoerceArgumentValues()} should be treated
instead as an _execution error_.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine for now. Ideally anywhere we raise a request error is definitely a request error.

Is the case this is covering against per-type input coercion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Essentially all of the input coercion says to "raise request error" because it doesn't really know it might be called during execution. When we're coercing argument values for a field we're already deep in execution so it's arguably too late to raise a request error. GraphQL.js treats this as a field error. I used "should" to allow for implementations that wish to perform this argument validation ahead of time (before commencing execution) - Grafast might choose to do this, for example - whilst still reflecting the standard way of doing this (field error) used in GraphQL.js.

@leebyron leebyron force-pushed the runtime-errors branch 3 times, most recently from 9ce49b4 to 0bbd856 Compare April 17, 2025 00:57
@leebyron leebyron merged commit 521ef5b into main Apr 17, 2025
9 checks passed
@leebyron leebyron deleted the runtime-errors branch April 17, 2025 01:02
@benjie
Copy link
Member Author

benjie commented Apr 17, 2025

Really nice edits, thanks Lee! I especially like the expansion and lifting of response position and think @robrichard will appreciate that too!

The one minor issue is moving the anchor tags below the title means the header will now not be visible when the (legacy) link is clicked. Not a huge deal, and it makes maintaining the spec easier. If we do this enough, might be worth adding a feature to spec-md, maybe something like:

### Handling Execution Errors

<!-- !! Previously: Handling Field Errors -->

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants